Support flutter in cross-links and add tests #1680
Conversation
| @@ -69,7 +69,7 @@ main(List<String> arguments) async { | |||
| logInfo("Generating documentation for '${config.topLevelPackageMeta}' into " | |||
| "${outputDir.absolute.path}${Platform.pathSeparator}"); | |||
|
|
|||
| Dartdoc dartdoc = await Dartdoc.withDefaultGenerators(config, outputDir); | |||
| Dartdoc dartdoc = await Dartdoc.withDefaultGenerators(config); | |||
There was a problem hiding this comment.
Style question: since you're creatine an instance here, I'd have expected a named factory constructor instead of a static; any reason for that?
EDIT: like withoutGenerators(..)
There was a problem hiding this comment.
Asynchronous factory (or normal) constructors don't work in Dart. dart-lang/sdk#23115
There was a problem hiding this comment.
Ah, yes, right! I didn't notice the await. Thanks for clarifying. 👍
| help: 'Library names to ignore.', splitCommas: true), | ||
| new DartdocOptionArgOnly<List<String>>('excludePackages', [], | ||
| help: 'Package names to ignore.', splitCommas: true), | ||
| // This could be a ArgOnly, but trying to not provide too many ways | ||
| // to set the flutter root. |
lib/src/html/html_generator.dart
Outdated
| // TODO(jcollins-g): Eliminate special casing for SDK and use config file. | ||
| if ((option.root['topLevelPackageMeta'].valueAt(dir) as PackageMeta) | ||
| .isSdk == | ||
| true) { |
There was a problem hiding this comment.
Wow, this is awkward formatting.
cc @munificent
There was a problem hiding this comment.
rearranged a bit to minimize the impact of dartfmt's choices here.
lib/src/model.dart
Outdated
| @@ -80,7 +81,7 @@ int byFeatureOrdering(String a, String b) { | |||
| } | |||
|
|
|||
| final RegExp locationSplitter = new RegExp(r"(package:|[\\/;.])"); | |||
| final RegExp substituteName = new RegExp(r"%([nv])%"); | |||
| final RegExp substituteNameVersion = new RegExp(r"%([bnv])%"); | |||
There was a problem hiding this comment.
Maybe single quotes here and for locationSplitter?
|
PTAL: the locking around creating the flutter repo was completely wrong and is now fixed. |
tool/grind.dart
Outdated
| RandomAccessFile _updateLock = | ||
| new File(pathLib.join(cleanFlutterDir.parent.path, 'lock')) | ||
| .openSync(mode: FileMode.WRITE); | ||
| _lockFuture = _updateLock.lock(); |
There was a problem hiding this comment.
You're using file locking here - is this because you expect multiple dartdoc grind processes to be running simultaneously?
Some of the Future / locking code could be simplified a bit by using a Completer. If the Completer global is null, create one and run the init code. Else, return myCompeter.future.
There was a problem hiding this comment.
I don't expect that to happen except accidentally on the command line, which I've done before.
Completers do indeed seem useful and better than this mess. Cleaned this up substantially and reuploaded, PTAL.
| @@ -66,7 +66,7 @@ final newLinePartOfRegexp = new RegExp('\npart of '); | |||
|
|
|||
| final RegExp quotables = new RegExp(r'[ "\r\n\$]'); | |||
|
|
|||
| /// Best used with Future<void>. | |||
| /// Best used with Future<Null>. | |||
| class MultiFutureTracker<T> { | |||
There was a problem hiding this comment.
Not for this PR, but there is a pool package (https://pub.dartlang.org/documentation/pool/1.3.4/), which may also do what you want here.
A batch of changes refining the cross-linking mechanism introduced in #1678. With this, I think it's ready for prime-time use.